duplicate detection via change store by rashmi-prithyani · Pull Request #144 · uber/submitqueue

rashmi-prithyani · 2026-03-17T05:29:02Z

Summary

Stacked on top of #152 (change store extension).

Detects duplicate land requests by reading per-URI claims from the new change store. The start controller writes a claim row for each URI on the request; validate reads the change store and rejects requests whose URIs overlap with another in-flight request in the same queue.

start.go — after persisting the request, claim each URI in the change store. INSERT IGNORE makes this idempotent on queue redelivery (same request_id retry is a no-op).
validate.go — query the change store for any row with an overlapping URI in the same queue (excluding self). For each unique candidate request_id returned, look up the request and skip terminal/orphan owners. If any live owner remains, reject as a user error naming the conflicting request_id.
Two single-table queries per check, no cross-store SQL joins.
Stop wrapping every storage error as Retryable; default is plain fmt.Errorf per the core/errs framework + refactor(logging): Do not double log and do not classify errors #134.
Wires changeStore into both controllers in example/server/orchestrator/main.go.

Behavior

Overlap (not full-set equality): the change store returns rows for any URI in the new request that matches a row from another in-flight request, so partial-overlap is rejected — addresses @sbalabanov's review comment.
Liveness: terminal-state owners and orphans (whose request row is gone) are skipped, so stale claims don't block new requests indefinitely.
Retries: start tolerates ErrAlreadyExists from request creation as a same-id replay; the change store's INSERT IGNORE is idempotent for the same reason.

Test Plan

Unit tests for start and validate cover happy path, overlap-with-live, overlap-with-terminal-skipped, overlap-with-orphan-skipped, multi-URI-same-owner deduped, and infra-error propagation.
Existing integration tests on extension/storage/mysql continue to pass after GetActiveByQueue removal.
Change store integration test (in add change store extension for per-URI request claims #152) exercises the MySQL impl against a real DB.

Issues

sbalabanov · 2026-04-16T21:57:00Z

+	activeRequests, err := c.store.GetRequestStore().GetActiveByQueue(ctx, request.Queue)
+	if err != nil {
+		coremetrics.NamedCounter(c.metricsScope, "process", "duplicate_check_errors", 1)
+		return errs.NewRetryableError(fmt.Errorf("failed to fetch active requests for duplicate check %s: %w", request.ID, err))


not every db error is ultimately retryable. See error framework and #134 for details.

Replace the in-Go scan over GetActiveByQueue with a server-side query against the change store. start writes URI claims; validate reads them. - start.go: claim each URI in the change store after persisting the request; INSERT IGNORE makes this idempotent on queue redelivery - validate.go: query change store for overlapping URIs in the same queue, loop unique candidate request_ids and check liveness via RequestStore.Get; skip orphans (ErrNotFound) and terminal owners - two single-table queries per check, no cross-store SQL joins - stop wrapping every storage error as Retryable; default is plain fmt.Errorf - main.go: wire changeStore into both controllers Addresses review comments on PR #144.

Introduce a dedicated extension that tracks (URI, request_id) claims with mutable metadata, scoped to queue. Foundation for cross-request URI overlap detection; no consumers in this commit. - entity/change_record.go: immutable identity (URI, RequestID), mutable metadata - extension/changestore/: ChangeStore interface, mysql impl, mock, schema - INSERT IGNORE on writes for retry idempotency on (uri, request_id) - FindOverlapping is single-table; callers do their own liveness check - integration test under test/integration/extension/changestore/mysql/ - e2e suite applies the change schema (harmless no-op until consumers land)

The e2e suite_test applies the change schema, but the bazel test target was missing //extension/changestore/mysql/schema in its data list, so runfiles didn't ship the .sql file and ApplySchema couldn't find it.

… API - schema: PRIMARY KEY (queue, uri, request_id) — queue-scoped lookups become PK-prefix scans and the table is shardable by queue. Comment in the schema explains why request_id stays in the PK (concurrent claims by different requests coexist as distinct rows; same-request retries collide on the PK). - schema: metadata JSON NOT NULL. The mysql impl writes '{}' for empty metadata so callers don't need to know about the column constraint. - interface: drop excludeRequestID from FindOverlapping. Callers that want to skip self filter the result by RequestID themselves. Documented Create's batch atomicity (single multi-row INSERT, all-or-nothing). - mysql: FindOverlapping query now leads with `WHERE queue = ?` to align with the new PK order. - entity: expanded RequestID/Queue field comments to explain their PK roles. - README: documents the new key shape and metadata semantics. - integration tests: drop the 4th arg, replace TestFindOverlapping_ExcludesSelf with a test that asserts the store does NOT exclude self, and add an assertion that empty metadata round-trips as '{}'.

Replace the in-Go scan over GetActiveByQueue with a server-side query against the change store. start writes URI claims; validate reads them. - start.go: claim each URI in the change store after persisting the request; INSERT IGNORE makes this idempotent on queue redelivery - validate.go: query change store for overlapping URIs in the same queue, loop unique candidate request_ids and check liveness via RequestStore.Get; skip orphans (ErrNotFound) and terminal owners - two single-table queries per check, no cross-store SQL joins - stop wrapping every storage error as Retryable; default is plain fmt.Errorf - main.go: wire changeStore into both controllers Addresses review comments on PR #144.

The previous interface assumed a SQL-style backend that gives batch atomicity and multi-key WHERE IN queries for free, which over-constrains alternatives like DynamoDB or Bigtable. Move to single-record, single-URI primitives that any backend can implement cheaply; callers loop when they have multiple URIs (typically 1-5 per request). - Create(record ChangeRecord) — was Create([]ChangeRecord). No batch atomicity contract; same-PK conflict is INSERT IGNORE on the mysql impl, callers retry whole loops to converge. - GetByURI(queue, uri) — was FindOverlapping(queue, []uris). Returns every ChangeRecord for the (queue, uri) pair; caller loops over its URIs and filters by RequestID for self-exclusion. - README and integration tests updated for the new shape.

Captures the design lesson from PR #152: interfaces should be shaped for the technology space (SQL, KV, document, queues, RPC, …), not for the one implementation we're adding. Lists common over-constraints to avoid (batch atomicity, multi-key queries, server-side filters, cross-entity transactions, strict ordering / exactly-once, sync low-latency assumptions) and proposes the test: 'could DynamoDB / Kafka / Bigtable / a remote RPC / an in-memory map satisfy this signature without contortion?'

Replace the in-Go scan over GetActiveByQueue with a server-side query against the change store. start writes URI claims; validate reads them. - start.go: claim each URI in the change store after persisting the request; INSERT IGNORE makes this idempotent on queue redelivery - validate.go: query change store for overlapping URIs in the same queue, loop unique candidate request_ids and check liveness via RequestStore.Get; skip orphans (ErrNotFound) and terminal owners - two single-table queries per check, no cross-store SQL joins - stop wrapping every storage error as Retryable; default is plain fmt.Errorf - main.go: wire changeStore into both controllers Addresses review comments on PR #144.

Mirrors the request-store pattern (RequestStore.UpdateState takes oldVersion + newVersion). The schema gains 'version INT NOT NULL'; ChangeRecord gains 'Version int32'; mysql impl writes/reads it. No UpdateMetadata method yet — added when the first metadata-enrichment writer arrives.

Replace the in-Go scan over GetActiveByQueue with a server-side query against the change store. start writes URI claims; validate reads them. - start.go: claim each URI in the change store after persisting the request; INSERT IGNORE makes this idempotent on queue redelivery - validate.go: query change store for overlapping URIs in the same queue, loop unique candidate request_ids and check liveness via RequestStore.Get; skip orphans (ErrNotFound) and terminal owners - two single-table queries per check, no cross-store SQL joins - stop wrapping every storage error as Retryable; default is plain fmt.Errorf - main.go: wire changeStore into both controllers Addresses review comments on PR #144.

github-actions · 2026-05-21T05:34:05Z

⚠️ Automatic stack rebase failed

This PR could not be automatically rebased after its base PR was merged. The rebase hit conflicts that need manual resolution.

To fix manually:

git fetch origin
git checkout rprithyani/validationControllerImpl
git rebase --onto origin/main ef5e65c0c2dae18890137d2247ae2ffc2c2329be rprithyani/validationControllerImpl
# resolve conflicts, then:
git push --force-with-lease

Then update this PR's base branch:

gh pr edit 144 --base main

Replace the in-Go scan over GetActiveByQueue with a server-side query against the change store. start writes URI claims; validate reads them. - start.go: claim each URI in the change store after persisting the request; INSERT IGNORE makes this idempotent on queue redelivery - validate.go: query change store for overlapping URIs in the same queue, loop unique candidate request_ids and check liveness via RequestStore.Get; skip orphans (ErrNotFound) and terminal owners - two single-table queries per check, no cross-store SQL joins - stop wrapping every storage error as Retryable; default is plain fmt.Errorf - main.go: wire changeStore into both controllers Addresses review comments on PR #144.

The store no longer excludes self via a request_id filter; the controller filters its own request_id out of the returned rows before doing liveness lookups. Adds two test cases that exercise the new responsibility (self-row filtered without a Get, self mixed with another live owner).

Adapts to the per-record / per-URI ChangeStore API: - start.claimURIs loops Create per URI; each call is independent and idempotent on PK conflict, so partial-failure retries converge. - validate.checkDuplicate iterates request URIs, calls GetByURI for each, filters out self, dedupes seen owners, and short-circuits on the first live duplicate. - Test cases restructured to map records by URI (since the controller now queries one URI at a time) and to exercise multi-URI requests.

Mirrors the request-store pattern (Request.Version starts at 1).

The change-store write now happens at the tail of validate.Process, after all checks (duplicate, merge, metadata) pass. Per-queue partition leasing makes the read-then-claim sequence within validate race-free. Trade-off: requests that fail validation no longer leave orphan claim rows in the change store. start becomes simpler — it persists the request and publishes; the change-store dependency is gone. - start.go / start_test.go: drop changeStore field, dependency, and tests - validate.go: add claimURIs() helper called after metadata fetch - validate_test.go: dup-detection table now expects Create on the success path (.AnyTimes() covers both success and rejection cases) - example/server/orchestrator/main.go: stop passing changeStore to start

start writes the change rows on entry; validate stays read-only and uses the change store + request store liveness check to detect duplicates. Trade-off: validation-failing requests leave behind change rows whose owner request will become terminal, which the controller-side liveness check filters out at duplicate-detection time. Reverses the previous move-claim-to-validate commit. The earlier-claim story trades simpler validate (read-only) for orphan claim rows (filtered, not blocking).

rashmi-prithyani commented Mar 17, 2026

View reviewed changes

Comment thread extension/storage/mysql/request_store.go Outdated

rashmi-prithyani commented Mar 17, 2026

View reviewed changes

Comment thread orchestrator/controller/validate/validate_test.go Outdated

rashmi-prithyani marked this pull request as ready for review March 17, 2026 05:34

rashmi-prithyani requested review from a team, behinddwalls and sbalabanov as code owners March 17, 2026 05:34

manjari25 reviewed Mar 17, 2026

View reviewed changes

Comment thread orchestrator/controller/validate/validate.go Outdated

Comment thread orchestrator/controller/validate/validate.go Outdated

Comment thread orchestrator/controller/validate/validate.go Outdated

rashmi-prithyani requested a review from manjari25 March 18, 2026 21:32

rashmi-prithyani force-pushed the rprithyani/validationControllerImpl branch 2 times, most recently from 28cb2b8 to 73c4ff9 Compare March 18, 2026 22:19

sbalabanov requested changes Apr 16, 2026

View reviewed changes

behinddwalls force-pushed the rprithyani/validationControllerImpl branch from 73c4ff9 to fb993a4 Compare May 7, 2026 03:06

behinddwalls changed the base branch from main to preetam/changestore-extension May 7, 2026 03:06

behinddwalls requested a review from sbalabanov May 7, 2026 03:06

This was referenced May 7, 2026

refactor(gateway): publish via TopicRegistry; align orchestrator topic names with keys #150

Merged

add change store extension for per-URI request claims #152

Merged

behinddwalls changed the title ~~implement duplication check in validation controller~~ duplicate detection via change store May 7, 2026

behinddwalls force-pushed the rprithyani/validationControllerImpl branch from fb993a4 to 4189a26 Compare May 7, 2026 03:19

github-actions Bot force-pushed the preetam/changestore-extension branch from 1f6589e to 6057c35 Compare May 7, 2026 22:36

github-actions Bot force-pushed the rprithyani/validationControllerImpl branch from 4189a26 to 3881108 Compare May 7, 2026 22:36

behinddwalls added 3 commits May 7, 2026 23:18

test/e2e: include changestore schema in bazel data deps

cb3306f

The e2e suite_test applies the change schema, but the bazel test target was missing //extension/changestore/mysql/schema in its data list, so runfiles didn't ship the .sql file and ApplySchema couldn't find it.

behinddwalls force-pushed the rprithyani/validationControllerImpl branch from 3881108 to 1ab7e81 Compare May 8, 2026 06:25

behinddwalls force-pushed the preetam/changestore-extension branch from 6057c35 to ff53ce6 Compare May 8, 2026 06:25

behinddwalls force-pushed the rprithyani/validationControllerImpl branch from 1ab7e81 to 32a3a1e Compare May 8, 2026 06:39

behinddwalls force-pushed the rprithyani/validationControllerImpl branch from 32a3a1e to 33ff1db Compare May 8, 2026 07:00

Merge branch 'main' into preetam/changestore-extension

ef5e65c

behinddwalls added 6 commits May 21, 2026 14:33

start.claimURIs: initialize ChangeRecord.Version to 1

9072853

Mirrors the request-store pattern (Request.Version starts at 1).

behinddwalls force-pushed the rprithyani/validationControllerImpl branch from 4b434ae to e5f2167 Compare May 21, 2026 21:36

behinddwalls changed the base branch from preetam/changestore-extension to main May 21, 2026 21:43

Merge branch 'main' into rprithyani/validationControllerImpl

81dddf1

behinddwalls approved these changes May 21, 2026

View reviewed changes

behinddwalls merged commit e004fc4 into main May 21, 2026
13 checks passed

github-actions Bot deleted the rprithyani/validationControllerImpl branch May 21, 2026 21:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

duplicate detection via change store#144

duplicate detection via change store#144
behinddwalls merged 14 commits into
mainfrom
rprithyani/validationControllerImpl

rashmi-prithyani commented Mar 17, 2026 •

edited by behinddwalls

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sbalabanov Apr 16, 2026

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented May 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

rashmi-prithyani commented Mar 17, 2026 • edited by behinddwalls Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Behavior

Test Plan

Issues

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sbalabanov Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented May 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rashmi-prithyani commented Mar 17, 2026 •

edited by behinddwalls

Loading